Skip to content

fix: incorrect link of flamegraph and URI #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
not-matthias wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: incorrect link of flamegraph and URI #41

not-matthias wants to merge 1 commit into from

Conversation

@not-matthias
Copy link
Member

@not-matthias not-matthias commented Jan 16, 2026 •
edited
Loading

We modified the logic to exclude the warmup, which caused this regression. We can only have 1 pair of SampleStart/Stop markers (generated by measurement_start/stop) for one benchmark.

Instead of moving the measurement_start/stop functions around to exclude the warmup, we now pass a flag that prevents adding the BenchmarkStart/Stop markers.

@not-matthias not-matthias requested a review from Copilot January 16, 2026 11:32
Copilot AI reviewed Jan 16, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the marker emission pattern for CodSpeed benchmarking by ensuring sample markers wrap around all benchmark repetitions, with benchmark start only being called once on the first repetition. Previously, start_benchmark was called on every repetition, which would emit multiple nested MARKER_TYPE_SAMPLE_START/END pairs. Now the markers are emitted in the correct format where a single sample start/end pair wraps around all benchmark iterations, with multiple benchmark start/stop markers within.

Changes:

  • Added IsFirstRepetition() helper method to check if a benchmark runner is on its first iteration
  • Modified start_benchmark call to only execute on the first repetition
  • Added MARKER_TYPE_SAMPLE_START marker emission at the beginning of start_benchmark()
  • Added MARKER_TYPE_SAMPLE_END marker emission at the beginning of end_benchmark()

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
google_benchmark/src/benchmark_runner.h Added IsFirstRepetition() method to check if benchmark is on first repetition
google_benchmark/src/benchmark.cc Modified condition to only call start_benchmark on first repetition
core/src/codspeed.cpp Added sample start/end markers around benchmark execution

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 16, 2026 •
edited
Loading

Merging this PR will degrade performance by 32.51%

⚡ 20 improved benchmarks
❌ 16 regressed benchmarks
✅ 156 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ Simulation DoubleTest[MyTemplatedFixture, double] 90.8 ns 61.7 ns +47.3%
⚡ Simulation BarTest[MyFixture] 90.8 ns 61.7 ns +47.3%
❌ Simulation BM_Template1_Capture[int_string_test] 60.6 ns 89.7 ns -32.51%
❌ Simulation BM_Capture_int[int_test] 61.1 ns 90.3 ns -32.31%
⚡ Simulation BM_Template2[int, double] 91.1 ns 61.9 ns +47.09%
❌ Simulation BM_Template1_Capture[two_type_test, int, double] 60.8 ns 90 ns -32.41%
⚡ Simulation FooTest[MyFixture] 90.8 ns 61.7 ns +47.3%
⚡ Simulation TestA[MyTemplate1, int] 90.8 ns 61.7 ns +47.3%
⚡ Simulation TestB[MyTemplate2, int, double] 90.8 ns 61.7 ns +47.3%
❌ Simulation BM_custom_args[1000] 90.8 ns 120 ns -24.31%
⚡ Simulation IntTest[MyTemplatedFixture, int] 90.8 ns 61.7 ns +47.3%
⚡ Simulation BM_Template1[int] 90.8 ns 61.7 ns +47.3%
❌ Simulation BM_Capture[int_test] 61.1 ns 90.3 ns -32.31%
❌ Simulation BM_Capture[int_string_test] 60.6 ns 89.7 ns -32.51%
❌ Simulation BM_with_args[1000] 90.8 ns 120 ns -24.31%
❌ Simulation BM_custom_args[100] 90.8 ns 120 ns -24.31%
❌ Simulation BM_with_args[100] 90.8 ns 120 ns -24.31%
⚡ Simulation BM_rand_vector 151.4 ns 122.2 ns +23.86%
❌ Simulation BM_custom_name_in_namespace 61.7 ns 90.8 ns -32.11%
❌ Simulation BM_with_multiple_args[100/200] 153.3 ns 182.5 ns -15.98%
... ... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

Comparing cod-2022-incorrect-link-of-uri-and-flamegraph (660b1f9) with main (56e942a)

Open in CodSpeed

@not-matthias not-matthias force-pushed the cod-2022-incorrect-link-of-uri-and-flamegraph branch from 0eff824 to e311da2 Compare January 16, 2026 13:27
@not-matthias not-matthias changed the title fix: emit sample markers; only call start benchmark in first iteration fix: incorrect URI<->flamegraph link Jan 16, 2026
@not-matthias not-matthias changed the title fix: incorrect URI<->flamegraph link fix: incorrect link of flamegraph and URI Jan 16, 2026
@not-matthias not-matthias force-pushed the cod-2022-incorrect-link-of-uri-and-flamegraph branch from e311da2 to 660b1f9 Compare January 16, 2026 13:36
GuillaumeLagrange
GuillaumeLagrange requested changes Jan 16, 2026
View reviewed changes
Copy link
Contributor

@GuillaumeLagrange GuillaumeLagrange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://codspeed.io/CodSpeedHQ/codspeed-cpp/branches/cod-2022-incorrect-link-of-uri-and-flamegraph?utm_source=github&utm_medium=comment-v2&utm_content=benchmark&uri=examples%2Fgoogle_benchmark_cmake%2Fpause_timing_bench.hpp%3A%3ABM_large_setup&runnerMode=Instrumentation&sectionId=benchmark-comparison-section-comparison-passed

Feels like there are still some issues.

According to the code, we should never see the expensive_operation w/ fibo here

@not-matthias
Copy link
Member Author

not-matthias commented Jan 16, 2026 •
edited
Loading

codspeed.io/CodSpeedHQ/codspeed-cpp/branches/cod-2022-incorrect-link-of-uri-and-flamegraph?utm_source=github&utm_medium=comment-v2&utm_content=benchmark&uri=examples%2Fgoogle_benchmark_cmake%2Fpause_timing_bench.hpp%3A%3ABM_large_setup&runnerMode=Instrumentation&sectionId=benchmark-comparison-section-comparison-passed

Feels like there are still some issues.

According to the code, we should never see the expensive_operation w/ fibo here

This seems to already have been the case 2 months ago: https://codspeed.io/CodSpeedHQ/codspeed-cpp/runs/692085eb175fc14ee5058b33?uri=examples%2Fgoogle_benchmark_cmake%2Fpause_timing_bench.hpp%3A%3ABM_large_setup&runnerMode=Instrumentation&sectionId=benchmark-comparison-section-single-run-benchmarks

I'll create a new ticket and fix it there

@not-matthias not-matthias force-pushed the cod-2022-incorrect-link-of-uri-and-flamegraph branch from 040ba03 to 660b1f9 Compare January 16, 2026 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@GuillaumeLagrange GuillaumeLagrange Awaiting requested review from GuillaumeLagrange

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@not-matthias @GuillaumeLagrange